Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Do not use Uris to uniquely identify Server Handles (1) #13540

Closed
wants to merge 1 commit into from
Closed

Do not use Uris to uniquely identify Server Handles (1) #13540

wants to merge 1 commit into from

Conversation

DonJayamanne
Copy link
Contributor

@DonJayamanne DonJayamanne commented May 20, 2023 •
edited
Loading

@DonJayamanne DonJayamanne force-pushed the removeServerId branch 7 times, most recently from 84f9c10 to 4fe028c Compare May 22, 2023 06:31
@DonJayamanne DonJayamanne changed the title WIP Do not use Uris to uniquely identify Server Handles May 22, 2023
@DonJayamanne DonJayamanne mentioned this pull request May 22, 2023
Closed
DonJayamanne
DonJayamanne commented May 22, 2023
View reviewed changes
src/kernels/errors/invalidRemoteJupyterServerUriHandleError.ts
public readonly handle: string,
public readonly extensionId: string,
public readonly serverId: string
public readonly serverHandle: { extensionId: string; id: string; handle: string },
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Handle Id and Handle are not unique, two extensions could have the same two values.

DonJayamanne
DonJayamanne commented May 22, 2023
View reviewed changes
src/kernels/errors/kernelErrorHandler.ts
@@ -249,37 +249,38 @@ export abstract class DataScienceErrorHandler implements IDataScienceErrorHandle
error: RemoteJupyterServerConnectionError,
errorContext?: KernelAction
) {
const info = await this.serverUriStorage.get(error.serverId);
const info = await this.serverUriStorage.get(error.serverHandle);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consistently use Serverhandle everwhere instead of generating an id called serverId and passing that around.

DonJayamanne
DonJayamanne commented May 22, 2023
View reviewed changes
src/kernels/execution/lastCellExecutionTracker.ts Outdated
private getStateKey(serverId: string) {
return `LAST_EXECUTED_CELL_${serverId}`;
private async getStateKey(serverHandle: JupyterServerProviderHandle) {
return `LAST_EXECUTED_CELL_${await computeServerId(serverHandle)}`;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have left this for backwards compatibility, computeServerid is not used in most places,
but this is an exception as we have stored state in older versions of extension and we should preserve that.

DonJayamanne
DonJayamanne commented May 22, 2023
View reviewed changes
src/kernels/jupyter/connection/jupyterPasswordConnect.ts
@@ -533,10 +537,9 @@ export class JupyterPasswordConnect implements IJupyterPasswordConnect {
* When URIs are removed from the server list also remove them from
*/
private onDidRemoveUris(uriEntries: IJupyterServerUriEntry[]) {
uriEntries.forEach((uriEntry) => {
const newUrl = addTrailingSlash(uriEntry.uri);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a bug, using Uri was never correct.

DonJayamanne
DonJayamanne commented May 22, 2023
View reviewed changes
src/kernels/types.ts
sendKernelTelemetry(this);
}
public static create(options: {
public static async create(options: {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to preserve the MRU for remote kernels, we must still generate the serverId, and thats an async function.
Hence this method is now async.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are the only two places we now use this (old) computeServerId

DonJayamanne
DonJayamanne commented May 25, 2023
View reviewed changes
src/kernels/jupyter/connection/serverUriStorage.ts
}
public async getAll(): Promise<IJupyterServerUriEntry[]> {
if (this.lastSavedList) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This caching was unnecessary, and a likely spot for bugs.

DonJayamanne
DonJayamanne commented May 25, 2023
View reviewed changes
src/kernels/jupyter/types.ts
* Unique ID using a hash of the full uri
*/
serverId: string;
uri?: string;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This property is now deprecated, it had a number of meanings in different contexts
Hence very buggy

@DonJayamanne DonJayamanne force-pushed the removeServerId branch 4 times, most recently from afa2c48 to cbfa7fb Compare May 26, 2023 23:34
@DonJayamanne DonJayamanne changed the title Do not use Uris to uniquely identify Server Handles Do not use Uris to uniquely identify Server Handles (1) May 28, 2023
@DonJayamanne DonJayamanne deleted the removeServerId branch November 28, 2023 00:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@DonJayamanne